Skip to content

Feature/interface signaller UI#1076

Merged
lukasrad02 merged 168 commits intohpi-sam:devfrom
lukasrad02:feature/interface-signaller-ui
Sep 23, 2025
Merged

Feature/interface signaller UI#1076
lukasrad02 merged 168 commits intohpi-sam:devfrom
lukasrad02:feature/interface-signaller-ui

Conversation

@lukasrad02
Copy link
Collaborator

@lukasrad02 lukasrad02 commented May 20, 2025

This PR adds a new modal that allows interface signallers to interact with the software

PR Checklist

Please make sure to fulfil the following conditions before marking this PR ready for review:

  • If this PR adds or changes features or fixes bugs, this has been added to the changelog
  • If this PR adds new actions or other ways to alter the state, test scenarios have been added.
  • I have the right to license the code submitted with this Pull Request under the mentioned license in the file LICENSE-README.md (i.e., this is my
    own code or code licensed under a license compatible to AGPL v3.0 or later, for exceptions look into LICENSE-README.md) and
    hereby license the code in this Pull Request under it.
    I certify that by signing off my commits (see In case of using third party code, I have given appropriate credit.
    We are using DCO for that, see here for more information.
  • If I have used third party code and I mentioned it in the code, I also updated the inspired-by-or-copied-from-list.html list to include the links.

lukasrad02 added 30 commits May 20, 2025 10:40
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
@ClFeSc
Copy link
Collaborator

ClFeSc commented Sep 9, 2025

I'm still unable to get the Transportorga to actually transport patients, but other than that I can say I'm happy with the changes in this PR.

@lukasrad02
Copy link
Collaborator Author

The Benötigte Fahrzeuge information is not available for B-Rooms that are targets for vehicle requests by e.g. a PA, despite this being logical and exactly what the note in the UI says about it (Benötigte Fahrzeuge um alle Patienten zu behandeln (PA) oder Anfragen zu erfüllen (B-Raum)).

We have already discussed this here and moved it to #1102. Since a reliable bugfix also needs #1094 fixed (for details, see my comment at the first issue), I would refrain from fixing it here and making this PR even larger.

Do you think it makes sense to adjust the description text in the UI as long as this information is not available on staging areas?

@ClFeSc
Copy link
Collaborator

ClFeSc commented Sep 11, 2025

The Benötigte Fahrzeuge information is not available for B-Rooms that are targets for vehicle requests by e.g. a PA, despite this being logical and exactly what the note in the UI says about it (Benötigte Fahrzeuge um alle Patienten zu behandeln (PA) oder Anfragen zu erfüllen (B-Raum)).

We have already discussed this here and moved it to #1102. Since a reliable bugfix also needs #1094 fixed (for details, see my comment at the first issue), I would refrain from fixing it here and making this PR even larger.

Do you think it makes sense to adjust the description text in the UI as long as this information is not available on staging areas?

Oops, sorry. Yes, probably a good idea to add a note in UI there if I have noticed it for a second time now ^^

Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
See hpi-sam#1102

Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
@lukasrad02
Copy link
Collaborator Author

I've just noticed that the Akzeptieren and Ablehnen buttons of the vehicle-request message don't have shortcuts associated with them.

Do you have a suggestion for what hotkeys to use? I would like to have something that does not conflict with other hotkeys, so that the IFS can for example switch to the staging area and ask for the number of vehicles available while the request is still open, so a simple Enter and Esc would not be possible.

I think the only option would be either F-keys or some combinations with Ctrl and/or Alt, if we still want to keep those buttons separate from the "Fertig" button.

Alternatively, I could remove the additional "Akzeptieren"/"Ablehnen" buttons and open a modal after "Fertig" was clicked. To make it clear that this radiogram needs additional information to complete it, the button could be renamed (e.g. "Antwort senden") and/or have a different color (e.g. yellow) in these cases. However, this would obviously need two key presses or button clicks (open modal, send answer) and would also require some bigger changes in the code, as the radiogram content (with the accept/deny buttons) is plugged into the frame with the done button, without the frame knowing whether there are additional actions inside.

@lukasrad02
Copy link
Collaborator Author

[…]

Oops, sorry. Yes, probably a good idea to add a note in UI there if I have noticed it for a second time now ^^

Temporarily removed staging area in 58ef848

@ClFeSc
Copy link
Collaborator

ClFeSc commented Sep 17, 2025

I've just noticed that the Akzeptieren and Ablehnen buttons of the vehicle-request message don't have shortcuts associated with them.

Do you have a suggestion for what hotkeys to use? I would like to have something that does not conflict with other hotkeys, so that the IFS can for example switch to the staging area and ask for the number of vehicles available while the request is still open, so a simple Enter and Esc would not be possible.

I think the only option would be either F-keys or some combinations with Ctrl and/or Alt, if we still want to keep those buttons separate from the "Fertig" button.

What about j and n for ja and nein, respectively? This should be unambiguous in the application and be somewhat logical without being too easy to do by accident, I think. Ah, I should've looked more closely, j is actually used and n is only two more information requests away from also being used. y and n for yes and no would work at the moment.

Otherwise, maybe Ctrl + Enter and Ctrl + Esc or Ctrl + Backspace? Ctrl + Esc is a little weird to hit on a normal keyboard. On the other hand, Backspace and Enter are right next to each other and could be mistakenly clicked.

With Ctrl we should look at how this gets handled on macOS with their Command key. Are there already shortcuts using Ctrl? I didn't find any.

Alternatively, I could remove the additional "Akzeptieren"/"Ablehnen" buttons and open a modal after "Fertig" was clicked. To make it clear that this radiogram needs additional information to complete it, the button could be renamed (e.g. "Antwort senden") and/or have a different color (e.g. yellow) in these cases. However, this would obviously need two key presses or button clicks (open modal, send answer) and would also require some bigger changes in the code, as the radiogram content (with the accept/deny buttons) is plugged into the frame with the done button, without the frame knowing whether there are additional actions inside.

I don't think shortcuts for two buttons warrant such massive changes.

Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
@lukasrad02
Copy link
Collaborator Author

I'm still unable to get the Transportorga to actually transport patients, but other than that I can say I'm happy with the changes in this PR.

I've investigated this again and now found two issues:

  1. The TO behavior does not manage its own region by default (as this would only make sense for TO behaviors added to patient trays, not for standalone TO regions). However, the TO region selector did not allow to add the own region. I've now fixed this in 590c629
  2. The TO needs a source for vehicles. Unfortunately, this mechanism is (1) separate from the resource request behavior we know from patient trays or staging areas, (2) unset by default, and (3) cannot be set from the IFS UI. As a quick fix, I've added a dedicated command to set the source in d525b46. An approach for a thorough solution is outlined in Unify vehicle requests between requestBehavior and managePatientTransportToHospitalBehavior #1110

@lukasrad02
Copy link
Collaborator Author

What about j and n for ja and nein, respectively? This should be unambiguous in the application and be somewhat logical without being too easy to do by accident, I think. Ah, I should've looked more closely, j is actually used and n is only two more information requests away from also being used. y and n for yes and no would work at the moment.

I'd rather not use single letters (or numbers) to not run into issues with future information requests or commands.

Otherwise, maybe Ctrl + Enter and Ctrl + Esc or Ctrl + Backspace? Ctrl + Esc is a little weird to hit on a normal keyboard. On the other hand, Backspace and Enter are right next to each other and could be mistakenly clicked.

With Ctrl we should look at how this gets handled on macOS with their Command key. Are there already shortcuts using Ctrl? I didn't find any.

No, there are no shortcuts using Ctrl yet and I would also not be able to test them on macOS systems, that's a fair point to consider.

What do you think about just using + (accept) and - (deny)? This does not need any combinations and should remain collision-free on the main modal (additional popups or modals deactivate all underlying hotkeys anyways)

Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
@ClFeSc
Copy link
Collaborator

ClFeSc commented Sep 17, 2025

[...]

I'd rather not use single letters (or numbers) to not run into issues with future information requests or commands.

Fair point.

[...]

No, there are no shortcuts using Ctrl yet and I would also not be able to test them on macOS systems, that's a fair point to consider.

Yes, me neither. I could ask some people with Macbooks to test it for me but this would be quite cumbersome.

What do you think about just using + (accept) and - (deny)? This does not need any combinations and should remain collision-free on the main modal (additional popups or modals deactivate all underlying hotkeys anyways)

+ and - sound good!

@christianschaeffer
Copy link
Member

No, there are no shortcuts using Ctrl yet and I would also not be able to test them on macOS systems, that's a fair point to consider.

I have a MacBook that I could use to test or or bring to HPI for your testing.

Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
@lukasrad02
Copy link
Collaborator Author

I now went for + and - (implemented in 5bb7417).

Thus, there is also no urgent need to test hotkeys on macOS, but I'll keep in in mind to test if we see each other anyways.

Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
@lukasrad02
Copy link
Collaborator Author

@ClFeSc if I didn't miss anything, all of your change requests and suggestions should now be addressed.

@lukasrad02 lukasrad02 requested a review from ClFeSc September 18, 2025 16:03
Copy link
Collaborator

@ClFeSc ClFeSc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two points remaining.

Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
Copy link
Collaborator

@ClFeSc ClFeSc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There aren't any test scenarios. Is this intended? If not, please add them, otherwise I'm happy with the changes and think we can merge.

@lukasrad02
Copy link
Collaborator Author

There aren't any test scenarios. Is this intended? If not, please add them, otherwise I'm happy with the changes and think we can merge.

There are test scenarios: hpi-sam/fuesim-digital-public-test-scenarios#17. But thx for the reminder. I want the submodule to always point to the commit on the main branch, so I cannot update it until the scenario PR is merged.

Let me quickly check if there have been any changes to the models during the review process (iirc, I actually changed a property name) and update the scenarios if necessary

@lukasrad02
Copy link
Collaborator Author

Okay, the only change was 2623a06, but I did this change before creating the test scenarios, so they're still up-to-date.

Signed-off-by: Lukas Radermacher <49586507+lukasrad02@users.noreply.github.com>
@lukasrad02 lukasrad02 merged commit 1d7619a into hpi-sam:dev Sep 23, 2025
14 checks passed
@lukasrad02 lukasrad02 deleted the feature/interface-signaller-ui branch September 23, 2025 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments